-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Kaniko builder #1168
Improve Kaniko builder #1168
Conversation
dgageot
commented
Oct 16, 2018
- Add more logs
- Simplify code
- Kaniko build can now be interrupted cleanly (Fixes Impossible to interrupt a Kaniko build #1145)
Codecov Report
@@ Coverage Diff @@
## master #1168 +/- ##
==========================================
- Coverage 42.18% 41.08% -1.11%
==========================================
Files 89 94 +5
Lines 3999 4126 +127
==========================================
+ Hits 1687 1695 +8
- Misses 2143 2262 +119
Partials 169 169
Continue to review full report at Codecov.
|
6929c7c
to
c26e30d
Compare
pkg/skaffold/build/kaniko/run.go
Outdated
@@ -97,6 +89,14 @@ func runKaniko(ctx context.Context, out io.Writer, artifact *latest.Artifact, cf | |||
return imageDst, nil | |||
} | |||
|
|||
func logLevel() logrus.Level { | |||
level := logrus.GetLevel() | |||
if level < logrus.InfoLevel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the special logic?
If users ask for debug logs, let them feel the onslaught of debug logs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the opposite I tried to achieve. Should be at least Info otherwise nothing is ever shown. I'll add a few tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops I didn't read closely enough. Maybe kaniko should output some useful information by default wdyt @priyawadhwa?
} | ||
|
||
func podTemplate(cfg *latest.KanikoBuild, args []string) *v1.Pod { | ||
return &v1.Pod{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
GenerateName: "kaniko", | ||
GenerateName: "kaniko-", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but WDYT about generateName for the secrets as well? I forget if I put it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, I don't think it's done
@@ -34,6 +34,10 @@ type artifactBuilder func(ctx context.Context, out io.Writer, tagger tag.Tagger, | |||
|
|||
// InParallel builds a list of artifacts in parallel but prints the logs in sequential order. | |||
func InParallel(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*latest.Artifact, buildArtifact artifactBuilder) ([]Artifact, error) { | |||
if len(artifacts) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't InParallel handle the single case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I don't think we can do multiple kaniko artifacts in parallel since we don't generateName for the secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inParallel can handle the single case but it's extra work on streams. More important than that, it loses a bit the colors. I couldn't fix it.
Building in // should work since the secret is shared by all jobs. It's defined at the builder level not the artifact level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Fixes GoogleContainerTools#1145 Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
c26e30d
to
5e65832
Compare